Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

xdp-forward: Introduce xdp-fwd-flowtable support #441

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

LorenzoBianconi
Copy link
Contributor

Introduce xdp-fwd-flowtable sample in order to perform XDP_REDIRECT between net_devices inserted in a netfilter flowtable.

Copy link
Member

@tohojo tohojo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few nits, mostly.

But as a bit of a bigger item, the tests don't cover the NAT code at all, so we basically have no way to check if all the fiddly packet header rewriting works. I think we should amend the tests to include each of the different NAT permutations. I know it's a lot of tedious work to test all the permutations, but I think it should be possible to parameterise the test functions so it becomes manageable. WDYT?

xdp-forward/xdp_flowtable.bpf.c Show resolved Hide resolved
xdp-forward/xdp_flowtable.bpf.c Show resolved Hide resolved
xdp-forward/xdp_flowtable.bpf.c Show resolved Hide resolved
xdp-forward/README.org Show resolved Hide resolved
xdp-forward/xdp-forward.c Show resolved Hide resolved
xdp-forward/xdp-forward.c Outdated Show resolved Hide resolved
xdp-forward/tests/test-xdp-forward.sh Outdated Show resolved Hide resolved
@LorenzoBianconi
Copy link
Contributor Author

LorenzoBianconi commented Sep 27, 2024

A few nits, mostly.

But as a bit of a bigger item, the tests don't cover the NAT code at all, so we basically have no way to check if all the fiddly packet header rewriting works. I think we should amend the tests to include each of the different NAT permutations. I know it's a lot of tedious work to test all the permutations, but I think it should be possible to parameterise the test functions so it becomes manageable. WDYT?

@tohojo what about doing something like:

  • perform dnat and snat at the same time changing IP ad port
  • load a "guard" ebpf program on the second veth that allows just selected ips and ports

In this way we can tests all the possible combinations at the same time, WDYT?

Introduce xdp-fwd-flowtable sample in order to perform XDP_REDIRECT
between net_devices inserted in a netfilter flowtable.
xdp-fwd-flowtable relies on bpf_xdp_flow_lookup kfunc in order to
perform the lookup of a given flowtable entry based on a fib tuple of
incoming traffic. At the moment we are able to offload just TCP or UDP
netfilter flowtable entries to the xdp layer. The user is supposed to
configure the flowtable separately.

Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
@LorenzoBianconi LorenzoBianconi force-pushed the xdp-flowtable branch 2 times, most recently from bbae4f7 to 6eef168 Compare September 27, 2024 17:38
@tohojo
Copy link
Member

tohojo commented Sep 30, 2024 via email

@LorenzoBianconi LorenzoBianconi force-pushed the xdp-flowtable branch 3 times, most recently from f112a0e to f898bf7 Compare October 2, 2024 13:46
@LorenzoBianconi
Copy link
Contributor Author

lore @.***> writes:

A few nits, mostly. > > But as a bit of a bigger item, the tests don't cover the NAT code at all, so we basically have no way to check if all the fiddly packet header rewriting works. I think we should amend the tests to include each of the different NAT permutations. I know it's a lot of tedious work to test all the permutations, but I think it should be possible to parameterise the test functions so it becomes manageable. WDYT? @tohojo what about doing something like: - perform dnat and snat at the same time changing IP ad port - load a program on the second veth that allows just selected ips and ports In this way we can tests all the possible combinations at the same time, WDYT?
Sure, that sounds reasonable (as long as we run it for both IPv4 and IPv6, of course) :)

I think we can do something even simpler, we can just filter out unexpected packets using some nft rules in the destination namesapce.

@LorenzoBianconi LorenzoBianconi force-pushed the xdp-flowtable branch 3 times, most recently from 9fe3158 to d3e5563 Compare October 2, 2024 16:30
Copy link
Member

@tohojo tohojo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple more nits :)

xdp-forward/README.org Show resolved Hide resolved
xdp-forward/tests/test-xdp-forward.sh Show resolved Hide resolved
xdp-forward/tests/test-xdp-forward.sh Outdated Show resolved Hide resolved
… userspace

Introduce the capability to load xdp-fw-flowtable sample to offload in
xdp the processing of sw netfilter flowtable.

Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
}
EOF

#xdp-forward load -f flowtable n0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, so upon seeing this, I realised there's a semantic difference between the flowtable XDP program and the fib XDP program: The latter uses bpf_redirect_map(), and explicitly sets up the map with only the interfaces passed on the command line. Whereas the flowtable program uses bpf_redirect(), and will redirect to any interface that the fib lookup succeeds against.

This is pretty inconsistent, and we should not have this difference only based on the mode the program is loaded in. Also, the way the fib program works is deliberate: the user should be able to select only a subset of interfaces to forward between (in the future I am planning to add an auto mode that uses XDP feature flags to automatically select compatible interfaces). So please change the flowtable program to behave the same as the other one.

And sorry for not realising this sooner. I did notice the difference in helpers, but didn't realise the implications until seeing this documentation change just now :(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants